-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement network plugin #7947
Implement network plugin #7947
Conversation
Hi @henry118. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
a4d65a3
to
04ce65c
Compare
99b6d71
to
de53df8
Compare
8ebc6ca
to
55917ce
Compare
Signed-off-by: Henry Wang <henwang@amazon.com>
@dmcgowan @mikebrow @MikeZappa87 PTAL, thanks! |
@henry118 excited to give this a review! This is blog worthy |
pluginName = "cni" | ||
) | ||
|
||
if os.Getenv("ENABLE_NETWORK_SRV") == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this is inspired by ENABLE_CRI_SANDBOXES. But do you think we cannot make this stable enough before releasing 1.7.0? My worry is that nobody use this if that is opt-in.
} | ||
|
||
// newCNINetConfSyncer creates cni network conf syncer. | ||
func newCNINetConfSyncer(confDir string, netPlugin cni.CNI, loadOpts []cni.Opt) (*cniNetConfSyncer, error) { | ||
func newCNINetConfSyncer(confDir string, netPlugin compat.CNI, loadOpts []compat.LoadOpt) (*cniNetConfSyncer, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we eventually use github.com/containerd/containerd/pkg/net
instead of github.com/containerd/containerd/pkg/net/compat
?
compat.CNI
is fine, but types like compat.LoadOpt
, compat.API
don't tell the intention much. So it may be better to rename the package or merge that into net
.
|
||
// cniAdaptor is created to adapt the APIs of network plugin to their | ||
// counterparts in go-cni. | ||
type cniAdaptor struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The adapter is for both g
and c
. Isn't cni.CNI and compat.CNI mostly compatible?
|
||
var dopts []cni.Opt | ||
for _, o := range opts { | ||
name := getFunctionName(o) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to avoid reflection if possible. Why do we need to know the name?
"manager": a.Manager(), | ||
"network": a.Network(), | ||
"id": a.ID(), | ||
}).Debugf("remove") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Debug
if ifname, ok := n.Labels()["ifname"]; ok { | ||
dopts = append(dopts, net.WithIFName(ifname)) | ||
} | ||
go c.asynchAttach(ctx, i, n, &wg, rc, dopts...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be errgroup?
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Implements network service plugin
This is patch that implements the network service plugin proposed by #7751
The main logic is implemented in
pkg/net
. In order to ease the integration with the existing CRI code,pkg/net/compat
was written to provide similar APIs asgo-cni
package.netapi_adaptor.go
was created in CRI to wrap the underlying implementations.This implementation does not change
go-cni
, and therefore limits its current scope to CRI code base. But the downside is that it contains similar codes that handles network configuration and results as ingo-cni
.Signed-off-by: Henry Wang henwang@amazon.com